Remove the interface function fullCollectNoStack from the gcinterface #16401
Conversation
|
Thanks for your pull request, @schveiguy! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#16401" |
|
Just so I can record the stuff I learned while researching this: The original D1 GC only called this branch of the code (what was the equivalent of The original I believe the idea was that since the gc is being removed anyway, and the only place in a single-threaded program where this GC is being terminated is from the main thread after It's kind of foolish in one sense -- if this is the case the main thread stack is tiny, and there are no other stacks to scan. Over the years as the code has evolved, comments were incorrectly added in the wrong place, making it seem like the current code is necessary, but I think it is not. I believe there is no harm in scanning additional roots. And since D1, the |
| @@ -8,5 +8,6 @@ struct S | |||
|
|
|||
| void main() | |||
| { | |||
| new S; | |||
| foreach(i; 0 .. 100) | |||
| new S; | |||
There was a problem hiding this comment.
maybe add a GC.collect(); here to force a collection then?
There was a problem hiding this comment.
I'm not seeing why we have to force one when it's going to collect at the end of the program anyway. The goal here is to make sure there is at least one piece of garbage that will trigger the error, and making 100 of them means at least one of those won't be referred to in stack/registers.
Probably could be 2, but 100 is cheap...
schveiguy
force-pushed
the
removeFullCollectNoStack
branch
from
ffdef39
to
8eac683
Compare
schveiguy
force-pushed
the
removeFullCollectNoStack
branch
from
8eac683
to
782e6ad
Compare
|
What's the deal with the coverage upload failures? Are those normal? |
|
No idea. Yes. |
|
I'm sick of the coverage upload failures. It should be removed, there are no acceptable solutions for public repositories to fix this issue. https://community.codecov.com/t/upload-issues-unable-to-locate-build-via-github-actions-api/3954 |
|
A question for reviewers, is a deprecation cycle needed for this? I seriously doubt anyone even thinks about using the GC interface. But it is there... |
That would be good |
thewilsonator
added
the
Needs Changelog
|
changelog added. |
schveiguy
removed
the
Needs Changelog
This is an experiment to see whether anything will fail if we completely remove the (seemingly useless/dangerous) fullCollectNoStack.OK, no longer an experiment. I think we should remove this function.
I probably should write a changelog...
See the investigation I did here: https://forum.dlang.org/post/[email protected]